-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rwork the token cleanup mechanism #719
Conversation
try { | ||
for (KubernetesNamespaceMeta namespaceMeta : namespaceFactory.list()) { | ||
List<Secret> secrets = doGetPersonalAccessTokenSecrets(namespaceMeta); | ||
for (int i = 1; i < secrets.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove all secrets or keep the last one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to keep the last one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we keep the very first one, should it be the last one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I mixed up, according to https://github.com/eclipse-che/che-server/pull/719/files#diff-c0fa2851d5bdc5b65a29d8ac5d7d71d4a961b03f526f43c50027e82b7948f775R257 we need to keep the first one - the newest.
/retest |
@vinokurig: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked on Che 7.91.0 with che-server PR image. There is only one token as expected.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SkorikSergey, tolusha, vinokurig The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build 3.17 :: server_3.x/357: Console, Changes, Git Data |
Build 3.17 :: sync-to-downstream_3.x/7694: Console, Changes, Git Data |
Build 3.17 :: get-sources-rhpkg-container-build_3.x/7686: server : 3.x :: Failed in 64104174 : BREW:BUILD/STATUS:UNKNOWN |
What does this PR do?
Rework the token secrets cleanup mechanism by fetching kubernetes secrets and deleting the redundant secrets directly, instead of fetching personal access tokens and then deleting the secrets according to the PAT objects.
Screenshot/screencast of this PR
What issues does this PR fix or reference?
https://issues.redhat.com/browse/CRW-7185
How to test this PR?
quay.io/eclipse/che-server:pr-719
See: only one personal access token item is present in the list
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedRelease Notes
Fix a bug when the list of PATs in the dashboard has redundant tokens if the refresh mode is enabled.
Reviewers
Reviewers, please comment how you tested the PR when approving it.